Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only raise error within union_relations for build/run sub-commands #607

Merged
merged 2 commits into from
Jun 15, 2022

Conversation

dbeatty10
Copy link
Contributor

@dbeatty10 dbeatty10 commented Jun 15, 2022

This is a:

  • documentation update
  • bug fix with no breaking changes
  • new functionality
  • a breaking change

All pull requests from community contributors should target the main branch (default).

Description & motivation

Fixes #606

Thanks to @erika-e for reporting the issue with dbt_utils.union_relations on dbt Cloud and @jeremyyeo for figuring out what was happening and how to solve it.

Based on the description in #606, I'm guessing slim CI or something similar was being used. Regardless, I think we have a fix that will support other use-cases too, like SQLFluff.

Checklist

  • I have verified that these changes work locally
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)
  • I have added an entry to CHANGELOG.md

Implementation

I considered several variations for the implementation.

Options for the boolean logic

flags.WHICH isn't very well documented, but it identifies the dbt subcommand that is executing. It looks like it is planned to be renamed in the future.

We know from Erika's error report that the problem is arising during 'generate', but it raises the question if more subcommands should be protected from this specific error too.

  1. flags.WHICH != 'generate'
  2. flags.WHICH not in ['generate']
  3. flags.WHICH not in ['compile', 'generate']
  4. flags.WHICH in ['run', 'build']
  5. flags.WHICH in ['test', 'run', 'build']

I chose 4. as the most targeted since we know that we want run and build to raise an error, but the rest of the commands might be debatable.

Options for readability

  1. Partial abstraction
    {%- set dbt_command = flags.WHICH -%}
    {% if dbt_command in ['run', 'build'] %}
    
  2. More full abstraction
    {%- set dbt_command = flags.WHICH -%}
    {%- set relevant_commands = ['run', 'build'] -%}
    {% if dbt_command in relevant_commands %}
    

Chose 1. as a balance of clarity and brevity.

Options for where to place the boolean logic

  1. Extend the existing if statement even further
    {% if dbt_command in ['run', 'build'] and (include | length > 0 or exclude | length > 0) and not column_superset.keys() %}
  2. Trim the existing if statement (since parts might not be necessary anymore)
    {% if dbt_command in ['run', 'build'] and not column_superset.keys() %}
  3. Wrap the existing error-raising section with{% if dbt_command in ['run', 'build'] %} ... {%- endif -%}
  4. Wrap only the line that actually raises the error with{% if dbt_command in ['run', 'build'] %}
    {%- if dbt_command in ['run', 'build'] -%}
        {{ exceptions.raise_compiler_error(error_message) }}
    {%- endif -%}
    

I'm feeling too risk averse for 2. (especially given #509 as the response to #505) 😅 But I do think 2. would work based on what I've seen so far.

Opted for 3. because it guards all the error-raising logic in a tidy wrapper and yields the most readable diff.

Future hopes

The union_relations macro is really cool because it handles the complex logic to union two relations that don't necessarily have the same columns. But it's grown organically and been fragile as a result (see here for a partial list of bug reports and fixes).

I'd love to see us:

  1. invest in consolidating a prioritized list of all the desired behaviors
  2. write applicable tests for each of those scenarios
  3. re-factor this macro to meet each of those use-cases.

@dbeatty10 dbeatty10 marked this pull request as ready for review June 15, 2022 19:28
@dbeatty10 dbeatty10 merged commit 5e43b59 into main Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

union_relations with exclude param on unmodified models not working with dbt docs generate in CI run
1 participant